feat: Add ServerTrace instrumentation hooks to the SSE server - #67
Open
keelerm84 wants to merge 3 commits into
Open
feat: Add ServerTrace instrumentation hooks to the SSE server#67keelerm84 wants to merge 3 commits into
keelerm84 wants to merge 3 commits into
Conversation
kinyoklion
approved these changes
Aug 5, 2026
keelerm84
marked this pull request as ready for review
August 5, 2026 15:46
kinyoklion
reviewed
Aug 6, 2026
| return fmt.Errorf("unexpected parameter to Encode: %v", ec) | ||
| // %T, not %v: an unexpected value must not have its contents -- which | ||
| // could include an event payload -- rendered into an error string that | ||
| // flows to WriteError consumers and logs. |
Member
There was a problem hiding this comment.
Suggested change
| // flows to WriteError consumers and logs. | |
| // logs the type (%T) and not the content, which may contain sensitive data. |
ServerTrace is a struct of optional callbacks modeled on net/http/httptrace.ClientTrace, exposed as Server.Trace: fields can be added without breaking implementers, info structs (not positional arguments) carry the payloads, and a nil struct or nil field disables that hook. The API is marked EXPERIMENTAL and for LaunchDarkly libraries only. This commit adds the contract and the Server-side reporting helpers; the next commit wires them through the handler and dispatch paths. Design points: - Callbacks fire synchronously on internal goroutines and must return promptly and never panic; SubscriberDropped fires on the dispatch goroutine, where an unrecovered panic would take down the process rather than one connection, so that helper contains panics and reports them through the Logger (rendering only the panic value's type, never its contents). - Handler-goroutine callbacks receive the subscriber's request context for telemetry correlation only. - The same instrumentation points feed Logger: DEBUG on subscriber add/remove and replay drain, WARN on a slow-subscriber drop. Log lines identify connections by an opaque per-Server subscriber id and never include the channel name, because channel names may contain credentials. - Measurement is gated on the consuming callback being set (shouldMeasureWrite, writeTraced, beginSubscription/sinceOrZero), so a Server with neither Trace nor Logger reads no clock and allocates nothing new on the write path. - Encode errors render unexpected values with %T, not %v, so payload contents cannot leak into error strings that reach WriteError consumers and logs.
Calls the hooks at every lifecycle point and makes what they report true under the teardown races the server already had: - Exit reasons: each read-loop exit records its reason; for Server-initiated closes, run() records closeReason before closing the subscriber's channel and the handler reads it only after observing that close (the channel close is the happens-before edge). When a Server close races the handler's own exit, reasons rank: buffer_overflow above everything (SubscriberDropped already promised a matching removal), write_error above the remaining Server reasons, any Server reason above the speculative client_closed and max_conn_time. - Teardown: the handler tears down in two defers. reportExit runs first and holds the consumer-reachable reporting; cleanup registers earlier so it still resolves the reason, unsubscribes, and emits SubscriberRemoved while a panic from a reporting callback is unwinding. reportedAdded is set before SubscriberAdded so even a panic inside that callback produces the balancing removal. SubscriberAdded fires before the subscription is registered, so no other callback can precede it. - Replay: batches report at batch level (count, summed payload bytes, DrainDuration through the end-of-batch flush, measured only when consumed). Every ReplayStarted gets exactly one ReplayFinished; a batch abandoned mid-drain reports Aborted, with a handler-local exit probe that keeps the common disconnect-races-sentinel case reported as completed. Aborted is documented best-effort: a Server shutdown draining an abandoned batch concurrently can make it read as completed. Concurrent drains themselves are safe -- receivers split discarded events until the channel closes. - Accounting: a jitter-parked event whose connection ends is reported discarded (connection_ended), so every event is accounted sent or discarded; the parking slot clears before the write so a panicking EventSent cannot double-report it. - Drops: a replay-batch enqueue that overflows the buffer reports SubscriberDropped exactly as trySend does; a handler racing Server.Close escapes the registration send instead of parking forever.
Covers every callback, all six removal reasons and their precedence under races, context propagation, replay batch accounting (count, bytes, durations, best-effort Aborted), the panic-teardown invariants (a panicking callback never leaks the subscription, strands a producer, or breaks the 1:1 Added/Removed pairing), zero-overhead gating, and a concurrency stress test of the pairing invariant. Includes the encoder %T error-format pin.
keelerm84
force-pushed
the
mk/sdk-2746/server-trace-hooks
branch
from
August 6, 2026 16:54
045766e to
1b907e8
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds
ServerTrace, an opt-in set of callbacks that the SSEServerinvokes at points inits lifecycle, so a consumer can observe per-connection and replay behavior. This is phase 1
of the eventsource server observability work; the OTel bridge that implements these hooks
lives in ld-relay. The library creates no spans and takes no new dependencies -- it only
invokes optional callbacks. The client
Streamis untouched.The design follows
net/http/httptrace.ClientTrace: a struct of optional function fields,exposed as a new
Server.Tracefield. Payloads are*Infostructs rather than positionalarguments so fields can be added compatibly. A nil
ServerTrace, or a nil field within it,disables the corresponding hook. The whole surface is marked
EXPERIMENTALand for use byLaunchDarkly libraries only.
Callbacks
SubscriberAddedSubscriberRemovedSubscriberAdded, with reason and durationSubscriberDroppedBufferSizeand is disconnected (dispatch goroutine, no context)EventSent/CommentSentEventDiscardedjitter_coalesce) or a parked event's connection ends (connection_ended)WriteErrorReplayStarted/ReplayFinishedReplayFinishedperReplayStartedContract highlights
SubscriberRemovedReasonhas six values. When aServer-initiated close races the handler's own exit, reasons rank deliberately:
buffer_overflowabove everything (aSubscriberDroppedthat fired promises a matchingremoval),
write_errorabove the remaining Server reasons, any Server reason above thespeculative
client_closed/max_conn_time. The mechanism is acloseReasonwritten bythe dispatch goroutine before it closes the subscriber's channel; the channel close is the
happens-before edge.
so they do not emit per-event
EventSent;ReplayFinishedcarries the count, summedpayload bytes, and a
DrainDurationthat includes the end-of-batch flush.Aborteddistinguishes an abandoned drain from a completed one. It is best-effort: ahandler-local exit probe keeps the common case accurate (a client that takes the full
payload and drops, racing the sentinel), while the rare Server-shutdown-drain race is
documented rather than coordinated away -- an earlier draft made it exact with a
cross-goroutine claim protocol, deliberately removed as not worth its reasoning burden.
leak the subscription, strand a Repository producer, or break the 1:1 Added/Removed
pairing (two-defer teardown).
SubscriberDroppedfires on the dispatch goroutine, wherea panic would kill the process, so that one is contained and reported through the Logger.
callback (or Logger) being set; an unobserved Server reads no clock and allocates nothing
new on the write path.
identify connections by an opaque subscriber id and never include the channel name (which
may contain credentials); encode errors render unexpected values by type only.
Logging
The same instrumentation points feed
Server.Logger, covering events that were previouslysilent:
[DEBUG]on subscriber add/remove and replay drain,[WARN]on a slow-subscriberdrop,
[ERROR]on a contained callback panic.Review notes
Three commits: the API surface and reporting helpers, the wiring through the handler and
dispatch paths, and the tests. A companion analysis of why the contract carries teardown
machinery at all (an observability API inherits the concurrency of the thing it observes)
and what was deliberately left out is in the branch history and available on request.